Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Improve cast performance on macos - Only load root TLS certificates with HTTPs rpc-url #6350

Merged
merged 1 commit into from
Nov 18, 2023

Conversation

RedaOps
Copy link
Contributor

@RedaOps RedaOps commented Nov 18, 2023

Hi, this is my first contribution to foundry so hello everyone 👋 ! The explanation for this simple fix is pretty long, so please bear with me 😅 , because performance is greatly increased.

I was looking at issue #6204 and noticed the profiler logs that @DaniPopes provided:
#6204 (comment)

Can reproduce as well. Profiling shows that 80% of the time is spent in rustls_native_certs::load_native_certs: https://share.firefox.dev/463tYIx

rustls_native_certs::load_native_certs will load the root TLS certificates of the system in order to use them when making HTTPs requests. This happens when we build our HTTP request provider for ethers_providers using reqwest inside the RuntimeClient (and coincidentally, where my fix is made). rustls_native_certs::load_native_certs is subsequently called inside the reqwest client builder (source) if tls_built_in_root_certs is set to true.

However, rustls_native_certs::load_native_certs is a pretty expensive function:

Load root certificates found in the platform’s native certificate store.
...
This function can be expensive: on some platforms it involves loading and parsing a ~300KB disk file. It’s therefore prudent to call this sparingly.

Source

I believe (and please correct me if I'm wrong, this is just a guess), that this part slows down cast bn and other rpc based commands on MacOS and not on Linux because of how MacOS stores the certificates in the Keychain module. Maybe loading the certificates from this module is more complex than how other systems handle loading certificates. Or maybe the rustls_native_certs crate is not optimised for this architecture. I could research this more but it seems out of scope for this issue.

Motivation

Performance improvement
closes #6204

Solution

The fix is really simple and should not compromise anything. It simply tells the reqwest client builder to not load TLS certificates if the jsonrpc node url is http and not https (most common use case, when using a local node) by setting tls_built_in_root_certs to false. This increases performance significantly for local cast requests (~9 times faster).

Keep in mind this doesn't only improve the performance of cast bn, but should improve the performance for all cast commands that use the reqwest client. Also, the performane for HTTPs requests stays the same, and is NOT improved.

Another solution which should also increase performance for HTTPs requests would be to only load the TLS certificate for the URL of the node we will be talking to, but this is more complex (not sure if even possible without loading all the certificates).

Benchmarks

Environment: M1 Macbook Pro - Sonoma 14.0

cast 0.2.0 (3e12d88 2023-11-17T00:33:21.466278000Z)

❯ time cast bn
0
cast bn  0.13s user 0.03s system 65% cpu 0.243 total

❯ time cast bn --rpc-url https://uk.rpc.blxrbdn.com     
18595977
cast bn --rpc-url https://uk.rpc.blxrbdn.com  0.13s user 0.03s system 19% cpu 0.854 total

tls_built_in_root_certs false with http - this PR

❯ time ./target/release/cast bn
0
./target/release/cast bn  0.01s user 0.01s system 76% cpu 0.026 total

❯ time ./target/release/cast bn --rpc-url https://uk.rpc.blxrbdn.com
18596090
./target/release/cast bn --rpc-url https://uk.rpc.blxrbdn.com  0.14s user 0.03s system 18% cpu 0.864 total

@RedaOps
Copy link
Contributor Author

RedaOps commented Nov 18, 2023

This change will be deprecated once the alloy providers are fully implemented since the RuntimeClient will be removed. Maybe this is something that can be ported to the new providers, or do they use a different client?

@mattsse
Copy link
Member

mattsse commented Nov 18, 2023

This change will be deprecated once the alloy providers are fully implemented since the RuntimeClient will be removed

they' still be running reqwest so we def want that fix

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, tysm!

this is most likely the reason for slow bn

let mut client_builder = reqwest::Client::builder().timeout(self.timeout);
let mut client_builder = reqwest::Client::builder()
.timeout(self.timeout)
.tls_built_in_root_certs(self.url.scheme() == "https");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, this always defaults to true but we only need it if the provider URL is https.

@mattsse mattsse merged commit 196fdb7 into foundry-rs:master Nov 18, 2023
19 checks passed
@drortirosh
Copy link

drortirosh commented Nov 18, 2023

was this supposed to improve the performance of cast? because it didn't.
on latest foundry (cast 0.2.0 (c948388 2023-11-18T00:19:39.244711000Z) on MacOs I get:

 $ time cast ci
31337

real	0m0.199s    <<< curl does this at <17ms
user	0m0.097s
sys	0m0.034s

@RedaOps
Copy link
Contributor Author

RedaOps commented Nov 18, 2023

was this supposed to improve the performance of cast? because it didn't.

on latest foundry (cast 0.2.0 (c948388 2023-11-18T00:19:39.244711000Z) on MacOs I get:

 $ time cast ci

31337



real	0m0.199s    <<< curl does this at <17ms

user	0m0.097s

sys	0m0.034s

You are running the version right before this was merged 😅

Commit c948388 (which you are running) is the commit right before the fix.

Can you update it to the new master and try again?

@RedaOps RedaOps deleted the cast-performance branch November 18, 2023 13:46
@DaniPopes
Copy link
Member

@drortirosh Versions are released on a nightly basis, so either build off of master branch with foundryup -b master or wait until tomorrow.

@RedaOps Awesome work btw :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cast is slow on OSX
4 participants